Conversation
|
Ohayo, sensei! WalkthroughAdds transaction finality handling (new field, parsing, wiring into waiters and configs), bumps multiple package versions to 1.7.2, updates WORLD_VERSION constant, and expands public ABI/types in example manifest JSON (new world_version and several types/functions). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as "User / CLI"
participant Options as "TransactionOptions"
participant Parser as "parse_finality_status"
participant TxnCfg as "TxnConfig / TxnAction"
participant Waiter as "TransactionWaiter"
participant Node as "StarkNet node"
CLI->>Options: build options (includes finality_status?)
Options->>Parser: parse_finality_status(status)
Parser-->>TxnCfg: TransactionFinalityStatus
TxnCfg->>Waiter: TransactionWaiter::new(...).with_tx_status(status)
Waiter->>Node: wait for tx with specified finality
Node-->>Waiter: receipt / status
Waiter-->>TxnCfg: return receipt/status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (6)
bin/sozo/src/commands/options/transaction.rs(6 hunks)crates/dojo/utils/src/tx/deployer.rs(1 hunks)crates/dojo/utils/src/tx/invoker.rs(3 hunks)crates/dojo/utils/src/tx/mod.rs(4 hunks)examples/simple/manifest_sepolia.json(12 hunks)examples/spawn-and-move/dojo_dev.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/spawn-and-move/dojo_dev.toml
🧰 Additional context used
🧬 Code graph analysis (2)
crates/dojo/utils/src/tx/deployer.rs (1)
crates/dojo/utils/src/tx/waiter.rs (1)
new(100-115)
crates/dojo/utils/src/tx/invoker.rs (1)
crates/dojo/utils/src/tx/waiter.rs (1)
new(100-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
🔇 Additional comments (12)
examples/simple/manifest_sepolia.json (7)
273-283: Ohayo sensei! The newworld_versionfunction looks good.The addition of the view function to the
IWorldinterface is clean and consistent with the pattern of other metadata queries (likeresourceandmetadata). No apparent issues here.
1563-1644: Note: Type definitions are duplicated across contract ABIs.The same type definitions (
Option<felt252>,M,Option<M>,u256,OtherType) appear in both the first contract (lines 1563–1644) and the third contract (lines 2213–2294).If each contract independently exposes these types in its public ABI, this duplication is necessary and correct. However, if these are shared library types, consider whether they should be deduplicated in the manifest structure. Verify this is intentional.
Also applies to: 2213-2294
1709-1724: Ohayo sensei! Newsystem_5function registered in both contracts.The addition of
system_5to theMyInterfaceis consistent across both contract ABIs (first contract lines 1709–1724 and third contract lines 2359–2374). The function signature withOtherTypeandMparameters looks correct.Also applies to: 2359-2374
1782-1897: Ohayo sensei! The newsystem_freefunction and supporting types are properly wired.The free function is consistently added across contracts with the correct signature: multiple
Optionand custom type inputs, returningOption<u32>. The prerequisite type definitions (SocialPlatform,PlayerSetting,PlayerSettingValue, etc.) are in place before the function is declared.Also applies to: 2432-2547
1961-1963: System lists correctly updated to include new functions.Both affected contracts (ns-c1 and ns2-c1) include
system_5,upgrade, andsystem_freein their system lists. The ordering is consistent.Also applies to: 2611-2613
3-4: Verify deployment artifact updates.World and contract addresses/class_hashes have been updated (sensei, this is expected for a release). The format of these values (0x-prefixed hexadecimal addresses and class hashes) looks valid for Starknet. Since these are autogenerated deployment artifacts, ensure they came from the official build pipeline and match the compiled contracts.
Also applies to: 1493-1494, 1967-1968, 2143-2144
2621-2650: Model and event class hashes updated consistently.The
class_hashvalues for models (ns-M,ns-ModelTest,ns2-M) and events (ns-E,ns-EH) have been bumped to reflect contract changes. This is expected for a manifest update and appears consistent.crates/dojo/utils/src/tx/deployer.rs (1)
91-93: LGTM - Finality status properly wired!The transaction waiter now correctly applies the configured finality status before awaiting the receipt. This aligns perfectly with the new finality handling introduced across the transaction utilities.
crates/dojo/utils/src/tx/invoker.rs (1)
68-70: LGTM - Consistent finality status application!All three code paths that wait for transaction receipts (single invoke, multicall with chunks, and multicall without chunks) now consistently apply the finality status configuration. Excellent consistency, sensei!
crates/dojo/utils/src/tx/mod.rs (2)
55-73: LGTM - Well-documented finality status field!The new
finality_statusfield is properly integrated with:
- Clear documentation explaining the rationale (node syncing issues)
- Sensible default value (
AcceptedOnL2)- Manual
Defaultimplementation that correctly initializes all fields
83-83: LGTM - TxnAction properly extended!The
Sendvariant now carriesfinality_status, maintaining consistency with theTxnConfigstructure.bin/sozo/src/commands/options/transaction.rs (1)
130-150: LGTM - Robust parsing logic!The
parse_finality_statusfunction is well-implemented with:
- Case-insensitive parsing via
to_uppercase()- Sensible default (
AcceptedOnL2) when None- Proper error handling for invalid inputs
| #[arg(long)] | ||
| #[arg(help = "The finality status to wait for. Since 0.14, the nodes syncing is sometime \ | ||
| not fast enough to propagate the transaction to the nodes in the \ | ||
| PRE-CONFIRMED state. The default is ACCEPTED_ON_L2. Available options are: \ | ||
| PRE-CONFIRMED, ACCEPTED_ON_L2, ACCEPTED_ON_L1.")] | ||
| #[arg(global = true)] | ||
| #[arg(default_value = "ACCEPTED_ON_L2")] | ||
| pub finality_status: Option<String>, |
There was a problem hiding this comment.
Fix inconsistency between help text and parser!
The help text documents the finality status values with hyphens (PRE-CONFIRMED, ACCEPTED-ON-L2, ACCEPTED-ON-L1), but the parser at line 142 expects underscores (PRE_CONFIRMED, ACCEPTED_ON_L2, ACCEPTED_ON_L1).
This will confuse users who copy the values from the help text and encounter parsing errors.
Apply this diff to fix the help text:
- #[arg(help = "The finality status to wait for. Since 0.14, the nodes syncing is sometime \
- not fast enough to propagate the transaction to the nodes in the \
- PRE-CONFIRMED state. The default is ACCEPTED_ON_L2. Available options are: \
- PRE-CONFIRMED, ACCEPTED_ON_L2, ACCEPTED_ON_L1.")]
+ #[arg(help = "The finality status to wait for. Since 0.14, the nodes syncing is sometime \
+ not fast enough to propagate the transaction to the nodes in the \
+ PRE_CONFIRMED state. The default is ACCEPTED_ON_L2. Available options are: \
+ PRE_CONFIRMED, ACCEPTED_ON_L2, ACCEPTED_ON_L1.")]🤖 Prompt for AI Agents
In bin/sozo/src/commands/options/transaction.rs around lines 67 to 74, the help
text lists finality status values with hyphens but the parser expects
underscores; update the help text examples to use underscores (PRE_CONFIRMED,
ACCEPTED_ON_L2, ACCEPTED_ON_L1) so they match the parser and the default value,
and ensure the descriptive sentence still reads correctly with the underscore
variants.
| #[test] | ||
| fn test_parse_finality_status() -> Result<()> { | ||
| matches!( | ||
| parse_finality_status(Some("PRE_CONFIRMED".to_string())), | ||
| Ok(TransactionFinalityStatus::PreConfirmed) | ||
| ); | ||
|
|
||
| matches!( | ||
| parse_finality_status(Some("ACCEPTED_ON_L2".to_string())), | ||
| Ok(TransactionFinalityStatus::AcceptedOnL2) | ||
| ); | ||
|
|
||
| matches!( | ||
| parse_finality_status(Some("ACCEPTED_ON_L1".to_string())), | ||
| Ok(TransactionFinalityStatus::AcceptedOnL1) | ||
| ); | ||
|
|
||
| matches!(parse_finality_status(None), Ok(TransactionFinalityStatus::AcceptedOnL2)); | ||
|
|
||
| assert!(parse_finality_status(Some("INVALID".to_string())).is_err()); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Critical: Test assertions are not working!
Lines 195-210 use the matches! macro but don't assert the result, meaning these test cases always pass regardless of whether the parsing succeeds or fails. The matches! expression evaluates but the boolean result is discarded.
Apply this diff to fix the test assertions:
#[test]
fn test_parse_finality_status() -> Result<()> {
- matches!(
+ assert!(matches!(
parse_finality_status(Some("PRE_CONFIRMED".to_string())),
Ok(TransactionFinalityStatus::PreConfirmed)
- );
+ ));
- matches!(
+ assert!(matches!(
parse_finality_status(Some("ACCEPTED_ON_L2".to_string())),
Ok(TransactionFinalityStatus::AcceptedOnL2)
- );
+ ));
- matches!(
+ assert!(matches!(
parse_finality_status(Some("ACCEPTED_ON_L1".to_string())),
Ok(TransactionFinalityStatus::AcceptedOnL1)
- );
+ ));
- matches!(parse_finality_status(None), Ok(TransactionFinalityStatus::AcceptedOnL2));
+ assert!(matches!(parse_finality_status(None), Ok(TransactionFinalityStatus::AcceptedOnL2)));
assert!(parse_finality_status(Some("INVALID".to_string())).is_err());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn test_parse_finality_status() -> Result<()> { | |
| matches!( | |
| parse_finality_status(Some("PRE_CONFIRMED".to_string())), | |
| Ok(TransactionFinalityStatus::PreConfirmed) | |
| ); | |
| matches!( | |
| parse_finality_status(Some("ACCEPTED_ON_L2".to_string())), | |
| Ok(TransactionFinalityStatus::AcceptedOnL2) | |
| ); | |
| matches!( | |
| parse_finality_status(Some("ACCEPTED_ON_L1".to_string())), | |
| Ok(TransactionFinalityStatus::AcceptedOnL1) | |
| ); | |
| matches!(parse_finality_status(None), Ok(TransactionFinalityStatus::AcceptedOnL2)); | |
| assert!(parse_finality_status(Some("INVALID".to_string())).is_err()); | |
| Ok(()) | |
| } | |
| #[test] | |
| fn test_parse_finality_status() -> Result<()> { | |
| assert!(matches!( | |
| parse_finality_status(Some("PRE_CONFIRMED".to_string())), | |
| Ok(TransactionFinalityStatus::PreConfirmed) | |
| )); | |
| assert!(matches!( | |
| parse_finality_status(Some("ACCEPTED_ON_L2".to_string())), | |
| Ok(TransactionFinalityStatus::AcceptedOnL2) | |
| )); | |
| assert!(matches!( | |
| parse_finality_status(Some("ACCEPTED_ON_L1".to_string())), | |
| Ok(TransactionFinalityStatus::AcceptedOnL1) | |
| )); | |
| assert!(matches!(parse_finality_status(None), Ok(TransactionFinalityStatus::AcceptedOnL2))); | |
| assert!(parse_finality_status(Some("INVALID".to_string())).is_err()); | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In bin/sozo/src/commands/options/transaction.rs around lines 193 to 215, the
test uses matches! without asserting its boolean result so failures are ignored;
update each standalone matches!(...) to assert!(matches!(...)) (or replace with
assert_eq!(parse_finality_status(...), Ok(...)) if preferred) for the three
explicit string cases and the None case so the test actually fails on mismatch,
leaving the existing
assert!(parse_finality_status(Some("INVALID".to_string())).is_err()) and Ok(())
unchanged.
2 participants
Automated changes by create-pull-request GitHub action
Summary by CodeRabbit
New Features
Chores